Note that the suggested API changes would not require a lot of new tests. An absolute minimum would do, and the existing federation upstream tests would provide most coverage.

Sounds good. When I first came to adding support for managing federation upstreams to terraform, I expected to be defining parameter types and resources. However, I was swayed by the existence of both Federation and Shovel types already present in rabbithole. I probably should have opened a discussion beforehand. I think adding a set of methods that operate explicitly on runtime parameters aligns well with the underlying RabbitMQ APIs.

The docs describe using vhost-scoped parameters to configure settings at run time. I use your suggested name of RuntimeParameter.

A possible API design for managing runtime parameters might look like this.

This mirrors the underlying HTTP API (except for the order of path params) and follows existing rabbithole API conventions. The corresponding HTTP API calls are shown in the comments.

This is very much influenced by the Federation and Shovel plugins, but perhaps there are other things to consider? There are already methods for managing Shovels.

type RuntimeParameter struct {
  Name      string                `json:"name"`
  Vhost     string                `json:"vhost"`
  Component string                `json:"component"`
  Value     RuntimeParameterValue `json:"value"`
}

// could also just use the go type directly or interface{}
type RuntimeParameterValue map[string]interface{}


// /api/parameters A list of all vhost-scoped parameters.
ListRuntimeParameters()
  => []RuntimeParameter, error

// /api/parameters/component   A list of all vhost-scoped parameters for a given component.
ListRuntimeParametersFor(component string)
  => []RuntimeParameter, error

ListRuntimeParametersIn(vhost, component string)
  => []RuntimeParameter, error

// /api/parameters/component/vhost/name  An individual vhost-scoped parameter.
GetRuntimeParameter(vhost, component, name string)
  => *RuntimeParameter, error

PutRuntimeParameter(vhost, component, name string, value RuntimeParameterValue)
  => error

DeleteRuntimeParameter(vhost, component, name string)
  => error

Thoughts?

The above API were shown as public methods. But separate API for managing runtime parameters and federation upstream might be redundant or confusing.

I think this is what you were getting at. I got it.

For example.

// federation.go
func (c *Client) GetFederationUpstream(vhost, upstreamName string) (param *RuntimeParameter, err error) {
    return getRuntimeParameter(vhost, "federation-upstream", &param)
}

// runtime_parameter.go
func getRuntimeParameter(vhost, name string, rec interface{}) err error {
  // ...
  executeAndParseRequest(c, req, &rec)
  // ...
}

Let me put something together for review.

The proposed API makes sense to me. Additional public functions that operate specifically on upstreams also make sense to me. Your example with GetFederationUpstream is indeed what I had in mind but getRuntimeParameter can be GetRuntimeParameter instead (so, public). Managing runtime parameters is a thing that operators might want to do and automate :)

Thank you for your willingness to invest more time into this PR!

Okay, please review these changes.

This is still WIP, but I wanted to check in before moving completing the changes.

Changes so far

1. Added an API for runtime (vhost-scoped) parameters. See this commit.

This allows you to work with parameter values containing arbitrary data. See this test.
You can use this API to manage runtime parameters directly and forget about the other APIs. But you will have to work directly with map[string]interface{}.

2. Added some V2 methods to federation.go for PUTting and GETting runtime parameters that have a component of federation-upstream.

I have only added a couple of temporary V2 methods to show one approach to using the runtime parameter API to manage different types of runtime parameters.

• PutFederationUpstreamV2
• GetFederationUpstreamV2

3. To avoid any naming confusion, I renamed FederationUpstream to FederationInfo.

This approach requires a struct to represent a strongly-typed parameter value and this mirrors the approach used for Shovels. Note the PopulateRuntimeParameter method. We need to pass the desired struct to the JSON decoder. Otherwise, you will have to do some form of mapping. This method was private to begin with, but I made it public.

Other approaches worth considering.

1. The parameter API could just return RuntimeParameter and the federation API would be responsble for mapping from map[string]interface{} to FederationInfo.

This is a common approach, but requires extra code to safely access map values. However, it would remove the need for the PopulateRuntimeParameter method, making the API more consistent. Seems like a good option.

This sort of thing.

  // inside GetFederationUpstream
  param, err := GetRuntimeParameter()

  info := &FederationInfo{
    Name: param.Name,
    Definition: FederationDefinition{} 
  }

  m := param.Value.(map[string]interface{})
  if v, ok := m["uri"].(string); ok {
    info.Definition.Uri = v
  }

  return info
  // etc

2. I don't think returning RuntimeParameter from the federation API itself is an option, since the end user would need to do some some of conversion.

3. However, in addition to 1, you could embed RuntimeParamter in FederationInfo. This would help to cement the use of runtime parameters for configuring federation, but is really only a semantic change. This uses field shadowing to ignore the embedded RuntimeParameter.Value field. I would be concerned that this behaviour could change at some point in the future though, but shadowing of nested fields is supported.

type FederationInfo struct {
  RuntimeParameter
  Value FederationDefinition `json:"value"`
}
// decodes to the following json
// {
//   "name": "temporary",
//   "vhost": "rabbit/hole",
//   "component": "federation-upstream",
//   "value": {
//     "uri": "amqp://127.0.0.1/%2f",
//     "expires": 0,
//     "message-ttl": 0,
//     "max-hops": 0,
//     "prefetch-count": 1000,
//     "reconnect-delay": 1,
//     "ack-mode": "on-confirm",
//     "trust-user-id": false,
//     "exchange": "",
//     "queue": ""
//   }
// }

There is probably a more elegant solution to all of this, but this seems pretty consistent with the overall API now.

Thoughts?

This looks close to what I had in mind. Constructing a federation upstream (info if you will) struct from a runtime parameter struct makes most sense to me. Embedding the parameter feels like a loss of "encapsulation" for no particular benefit.

I personally think that FederationUpstream is the best name. "Federation info" is not very specific since there are like three "parts" that make up a federated entity: an upstream, a link, and the entity itself (some may even add a policy), and the struct clearly only covers the first part.

Thanks 🙏

Thank you so much! This is a great addition to the library.